Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[universal] - Issue universal config change for non-root default codespace user and installing google chrome browser reuse sandboc to run puppeteer cli in universal image #1287

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Kaniska244
Copy link
Contributor

Ref: actions/runner-images#10936

Dev container name:

Universal

Description:

This PR change is made to force 1001 as UID for the non-root default codespace user in the universal image due changes in GitHub runner for larger runners to force 1001 as UID for the runner user.

Changelog:

  • Updated devcontainer.json file to force 1001 as UID for codespace user in universal image & removed the GID value so that its picked up as automatic.

Checklist:

  • Checked that applied changes work as expected.

@Kaniska244 Kaniska244 marked this pull request as ready for review January 22, 2025 16:21
@Kaniska244 Kaniska244 requested a review from a team as a code owner January 22, 2025 16:21
@Kaniska244
Copy link
Contributor Author

Hi @ddoyle2017 ,

Tested another workaround for this instead of forcing the UID 1001 as that was increasing the size of the image considerably.

  • Setting "updateRemoteUserUID" flag to false so that UID & GID aren't replaced, so that the UID & GID of codespace user remains 1000.

  • Adding "postStartCommand" to change the ownership of /workspaces/images folder to the same as of the codespace user, not using "postCreateCommand" option as its already used in other sub-features in universal image. This folder is directly mounted from the local machine containing the images repo sources.

  • Tests are fine with this. Also tested locally in a codespace along with the devcontainer CI pipeline.

Would you kindly review the same. Please let me know in case of any concern.

With Regards,
Kaniska

Copy link
Contributor

@ddoyle2017 ddoyle2017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Can we revert the puppeteer --no-sandbox changes and rerun the Actions?

@Kaniska244
Copy link
Contributor Author

Kaniska244 commented Jan 28, 2025

Hi @ddoyle2017 ,

I have tested one fix for the puppeteer cli issue. Uploaded the fix. Would you kindly review the same & let me know in case of any concern.
Please note the change is only in the test script as the purpose of the test is to demonstrate that its possible to run a puppeteer node app in the universal container.

PFB the details:

  • The actual issue was that there was no sandbox available to launch the browser when runner image is on ubuntu-24.04.
  • Downloaded google chrome amd64-deb variant latest stable version & installed the same in the container.
  • Chrome is shipped with a default sandbox, copied the same & populated this CHROME_DEVEL_SANDBOX variable which is used by the puppeteer by default.
  • All tests are fine with this & this appears to be the safest approach compared to the other approaches which would require to launch the browser directly in the container without the sandbox or modifying the kernel to allow unprivileged access.

Ref - https://github.com/puppeteer/puppeteer/blob/88ef09c6ccc27d024972725c728a9db50f010f36/docs/troubleshooting.md#setting-up-chrome-linux-sandbox

With Regards,
Kaniska Sengupta

Copy link
Contributor

@ddoyle2017 ddoyle2017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a reasonable work around for the Puppeteer issues with Ubuntu 24.04! Can you add a comment above both your changes to explain what issue they're solving?

@@ -103,7 +103,8 @@
],
"remoteUser": "codespace",
"containerUser": "codespace",

"updateRemoteUserUID": false,
"postStartCommand": "sudo chown -R 1000:1000 /workspaces/images/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I know why this need to be a postStartCommand?
postStartCommand re runs on every time container is restarted. We need to do this work only once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eljog ,
Indeed it would be executed during every restart. Initially I saw that as part of configurations in one of the features of universal image, git-lfs has already got postCreateCommand used in devcontainer-feature.json. So I avoided the postCreateCommand option & opted for postStartCommand.
However, I have checked now that for a devcontainer configuration if the same lifecycle hook such as postCreateCommand is used in both devcontainer-feature.json & devcontainer.json, then the commands provided in the devcontainer-feature.json would be executed first followed by the commands in devcontainer.json. So I changed that & tested the same now. Kindly let me know in case of any further concerns on this.

With Regards,
Kaniska


# export CHROME_DEVEL_SANDBOX env variable
export CHROME_DEVEL_SANDBOX=/usr/local/sbin/chrome-devel-sandbox
cd /workspaces/images/src/universal/test-project/
yarn
check "run-puppeteer" node puppeteer.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this command work on the container when the user runs it?
Or is it passing here because the additional setup done above, before running the check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eljog ,

My idea was that the main objective behind this particular test "run-puppeteer" was to demonstrate that puppeteer cli library can be used in a node app & the same cane be executed in the universal image. So I changed it in the test.sh alone considering that & also due to the fact that as part of the solution I am downloading & installing google chrome in the image which increases the image size if done as part of the build.

Now I have changed that. Following points are needed to be considered for this change.

  • Made change in Dockerfile to install three more libraries which are required to install chrome.
  • Made change in install.sh of the local feature setup-user to install chrome & placing the sandbox in the desired location. Please let me know if I should introduce a new local feature for this instead.
  • Made change in devcontainer.json to configure "remoteEnv" tag for the CHROME_DEVEL_SANDBOX variable with the sandbox location which is used by the puppeteer library while launching the browser.
  • Size of the image has now increased considerably due to the chrome installation which is still within the configured threshold but only by about 64 MB. With the previous change the gap was about 422 MB.

Kindly let me know in case of any concerns on this.

With Regards,
Kaniska Sengupta

Copy link
Member

@eljog eljog Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is more about the user experience here.
If we don't install chrome on the image, will the puppeteer scenarios start breaking for them from the new version?
Are the users expected to manually install the setup each time, to get things working? Not sure if that's the expectation on using universal images.

Tagging other maintainers for their thoughts. @chrmarti @bamurtaugh

Copy link
Contributor Author

@Kaniska244 Kaniska244 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is more about the user experience here. If we don't install chrome on the image, will the puppeteer scenarios start breaking for them from the new version? Are the users expected to manually install the setup each time, to get things working? Not sure if that's the expectation on using universal images.

Tagging other maintainers for their thoughts. @chrmarti @bamurtaugh

Hi @eljog ,

Actually right now in the image built on top of ubuntu-24.04 runner image in CI, no sandbox is available to reuse for puppeteer node app which forces us to install chrome as it comes with sandbox & also due to enhanced security feature of ubuntu-24.04, its not possible to create sandbox without enabling user namespace cloning which still has some vulnerabilities.
Ref:- https://github.com/puppeteer/puppeteer/blob/88ef09c6ccc27d024972725c728a9db50f010f36/docs/troubleshooting.md#setting-up-chrome-linux-sandbox

puppeteer/puppeteer#12818 (comment)

With Regards,
Kaniska

@ddoyle2017
Copy link
Contributor

hey @Kaniska244, so @eljog and I had a discussion about how to move forward with the release and I think we need to reorganize your changes for Puppeteer.

Currently, your Chrome workaround works for our CI test, but Puppeteer will still be broken for users of this image. We should apply your updates to the image as a whole (not just in test.sh), then see if that fixes the issue 👍

@Kaniska244
Copy link
Contributor Author

hey @Kaniska244, so @eljog and I had a discussion about how to move forward with the release and I think we need to reorganize your changes for Puppeteer.

Currently, your Chrome workaround works for our CI test, but Puppeteer will still be broken for users of this image. We should apply your updates to the image as a whole (not just in test.sh), then see if that fixes the issue 👍

Hi @ddoyle2017 ,

I have made the changes according to the comments & all tests are fine with this. Would you kindly review the latest change.
PFB the details of the change.

  • Changed the postStartCommand option to postCreateCommand as per the review comments.
  • Made change in Dockerfile to install three more libraries which are required to install chrome.
  • Made change in install.sh of the local feature setup-user to install chrome & placing the sandbox in the desired location. Please let me know if I should introduce a new local feature for this instead.
  • Made change in devcontainer.json to configure "remoteEnv" tag for the CHROME_DEVEL_SANDBOX variable with the sandbox location which is used by the puppeteer library while launching the browser.
  • Size of the image has now increased considerably due to the chrome installation which is still within the configured threshold but only by about 64 MB. With the previous change the gap was about 422 MB.
  • Added comments on top of the changes for more clarity.

With Regards,
Kaniska

@Kaniska244 Kaniska244 changed the title [universal] - Issue universal config change for non-root default codespace user in universal image [universal] - Issue universal config change for non-root default codespace user and installing google chrome browser reuse sandboc to run puppeteer cli in universal image Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants